Hi @RyanDevlin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/assign @damemi @ingvagabund

Could you please review my PR?

Specifically @damemi I made some changes to the TopologySpreadConstraint strategy to avoid checking for node affinity and taints twice (see line 290 of topologyspreadconstraint.go). I'm fairly certain my changes didn't effect the functionality of the algorithm, but I wanted to double check with you.

@RyanDevlin: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-descheduler-verify-master b7daf8e link /test pull-descheduler-verify-master
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

@RyanDevlin It seems that you forgot to run make verify before pushing commits.

@RyanDevlin #551 is more about deciding what's the average number of duplicates per node rather than checking if a pod can be evicted. The issue is about avoiding unnecessary evictions in the first place.

@damemi I can't figure this one out. I only see ./hack/update-generated-conversions.sh not ./hack/update-conversions.sh. I ran ./hack/update-conversions.sh just in case, and then make verify and it's still failing. I also tried running make clean, make gen, and then make verify and it's still reporting errors. Any thoughts?

@ingvagabund In the description of #551 it says "Take at least node taints (e.g. taints repealing pods from master nodes) into account when selecting pods for eviction. If possible, take node selector into account as well". I'm a bit confused how that differs from what my feature does?

Since RemoveDuplicas filters pods by with the Evicatable function prior to calculating duplicates, would this feature not achieve the behavior desired in #551?

@damemi I can't figure this one out. I only see ./hack/update-generated-conversions.sh not ./hack/update-conversions.sh. I ran ./hack/update-conversions.sh just in case, and then make verify and it's still failing. I also tried running make clean, make gen, and then make verify and it's still reporting errors. Any thoughts?

Sorry, the script should be ./hack/update-generated-conversions.sh (note that you need to git commit the changes before make verify will pass after running the update script)

@damemi On my end, I think the problem is that the output of ./hack/update-generated-conversions.sh produces files with errors. The sigs.k8s.io/descheduler/pkg/ directory created by the script has zz_generated.conversion.go files with references to *DeschedulerPolicy that it says is undeclared.

Running make clean and then make gen, or make clean and ./hack/update-generated-conversions.sh still produces files with errors in them. This later causes my make verify to fail. I'm not sure how to fix that.

@RyanDevlin that's odd, I checked out your branch and had no issues:

$ git fetch git@github.com:RyanDevlin/descheduler.git nodeFit
remote: Enumerating objects: 109, done.
remote: Counting objects: 100% (80/80), done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 43 (delta 32), reused 40 (delta 31), pack-reused 0
Unpacking objects: 100% (43/43), 2.67 MiB | 6.56 MiB/s, done.
From github.com:RyanDevlin/descheduler
 * branch                nodeFit    -> FETCH_HEAD

$ git checkout FETCH_HEAD
Note: switching to 'FETCH_HEAD'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at c17141202 Working nodeFit feature

$ make verify
ls: _output/bin/golangci-lint: No such file or directory
./hack/verify-gofmt.sh
./hack/verify-vendor.sh
go: downloading k8s.io/apiserver v0.21.0
go: downloading k8s.io/client-go v0.21.0
go: downloading k8s.io/component-base v0.21.0
go: downloading k8s.io/apimachinery v0.21.0
go: downloading k8s.io/api v0.21.0
go: downloading k8s.io/component-helpers v0.21.0
go: downloading k8s.io/code-generator v0.21.0
Vendor Verified.
Removing ./hack/../_tmp/kube-vendor.V90L2b
curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b ./_output/bin v1.30.0
golangci/golangci-lint info checking GitHub for tag 'v1.30.0'
golangci/golangci-lint info found version: 1.30.0 for v1.30.0/darwin/amd64
golangci/golangci-lint info installed ./_output/bin/golangci-lint
./_output/bin/golangci-lint run
helm lint ./charts/descheduler
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /Users/mdame/Downloads/cluster-bot-2021-04-29-174456.kubeconfig
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /Users/mdame/Downloads/cluster-bot-2021-04-29-174456.kubeconfig
==> Linting ./charts/descheduler

1 chart(s) linted, 0 chart(s) failed
./hack/verify-spelling.sh
./hack/verify-toc.sh
./hack/verify-conversions.sh
Generated output differs:
diff -Naupr pkg/api/v1alpha1/zz_generated.conversion.go ./hack/../_tmp/kube-verify.ysXwIZ/descheduler/pkg/api/v1alpha1/zz_generated.conversion.go
--- pkg/api/v1alpha1/zz_generated.conversion.go	2021-04-21 10:09:28.000000000 -0400
+++ ./hack/../_tmp/kube-verify.ysXwIZ/descheduler/pkg/api/v1alpha1/zz_generated.conversion.go	2021-04-30 16:37:07.000000000 -0400
@@ -294,6 +294,7 @@ func autoConvert_v1alpha1_StrategyParame
 	out.ThresholdPriority = (*int32)(unsafe.Pointer(in.ThresholdPriority))
 	out.ThresholdPriorityClassName = in.ThresholdPriorityClassName
 	out.LabelSelector = (*v1.LabelSelector)(unsafe.Pointer(in.LabelSelector))
+	out.NodeFit = in.NodeFit
 	return nil
 }
 
@@ -313,6 +314,7 @@ func autoConvert_api_StrategyParameters_
 	out.ThresholdPriority = (*int32)(unsafe.Pointer(in.ThresholdPriority))
 	out.ThresholdPriorityClassName = in.ThresholdPriorityClassName
 	out.LabelSelector = (*v1.LabelSelector)(unsafe.Pointer(in.LabelSelector))
+	out.NodeFit = in.NodeFit
 	return nil
 }
 
Generated conversions verify failed. Please run ./hack/update-conversions.sh
make: *** [verify-gen] Error 1

$ ./hack/update-generated-conversions.sh 
$ git diff
diff --git a/pkg/api/v1alpha1/zz_generated.conversion.go b/pkg/api/v1alpha1/zz_generated.conversion.go
index 90351de35..1b91f8f50 100644
--- a/pkg/api/v1alpha1/zz_generated.conversion.go
+++ b/pkg/api/v1alpha1/zz_generated.conversion.go
@@ -294,6 +294,7 @@ func autoConvert_v1alpha1_StrategyParameters_To_api_StrategyParameters(in *Strat
        out.ThresholdPriority = (*int32)(unsafe.Pointer(in.ThresholdPriority))
        out.ThresholdPriorityClassName = in.ThresholdPriorityClassName
        out.LabelSelector = (*v1.LabelSelector)(unsafe.Pointer(in.LabelSelector))
+       out.NodeFit = in.NodeFit
        return nil
 }
 
@@ -313,6 +314,7 @@ func autoConvert_api_StrategyParameters_To_v1alpha1_StrategyParameters(in *api.S
        out.ThresholdPriority = (*int32)(unsafe.Pointer(in.ThresholdPriority))
        out.ThresholdPriorityClassName = in.ThresholdPriorityClassName
        out.LabelSelector = (*v1.LabelSelector)(unsafe.Pointer(in.LabelSelector))
+       out.NodeFit = in.NodeFit
        return nil
 }

With the only change being those 2 lines of out.NodeFit = in.NodeFit. Maybe double check that you are rebased onto the upstream/master branch and manually delete the _output and _tmp directories before running hack/update-generated-conversions.sh

@damemi I made sure I was rebased on the upstream master, deleted _tmp/, _output/, and sigs.k8s.io/, and re-ran the script. I'm still getting errors in the generated files. I even re-cloned my repo just to make sure some leftover artifact wasn't giving me trouble. Am I missing generated files? This is what I have in my generated directory:

rdevlin@localhost:~/go/src/sigs.k8s.io/descheduler$ tree sigs.k8s.io/descheduler/pkg/
sigs.k8s.io/descheduler/pkg/
├── api
│   ├── v1alpha1
│   │   ├── zz_generated.conversion.go
│   │   ├── zz_generated.deepcopy.go
│   │   └── zz_generated.defaults.go
│   └── zz_generated.deepcopy.go
└── apis
    └── componentconfig
        ├── v1alpha1
        │   ├── zz_generated.conversion.go
        │   ├── zz_generated.deepcopy.go
        │   └── zz_generated.defaults.go
        └── zz_generated.deepcopy.go

5 directories, 8 files

@ingvagabund In the description of #551 it says "Take at least node taints (e.g. taints repealing pods from master nodes) into account when selecting pods for eviction. If possible, take node selector into account as well". I'm a bit confused how that differs from what my feature does?

Since RemoveDuplicas filters pods by with the Evicatable function prior to calculating duplicates, would this feature not achieve the behavior desired in #551?

#551 is focused mainly for adjusting https://github.com/kubernetes-sigs/descheduler/blob/master/pkg/descheduler/strategies/duplicates.go#L107. As described in the description:

In case there's a cluster with 3 master nodes and 3 workers and a workload (owned by an RC) is uniformly
distributed among worker nodes (the expected distribution), the strategy still evicts pods of the workload
since it takes master nodes into account as well. E.g. 12 pods, 4 pods bound to each worker node, 6 pods
are evicted (6 nodes, 12 pods, 2 per node) which always end up on the same worker nodes again.

Currently, all 6 nodes are taken into account. So the average number of pods per node is 2. The idea is to adjust nodeCount to 3 (filter out masters which can not run evicted pods). With your PR the nodeCount is still 3 and there are two other worker nodes which can run the evicted pods. So instead of evicting 0 pods (since the pods are evenly distributed among the worker nodes), the strategy will still evict 6.

@ingvagabund I see what you mean, thanks for that link! I'll remove #551 from the description of the PR.

@damemi I'm still unable to resolve my generated conversions issue, but I have some more information if it helps. You can see the latest build logs here: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_descheduler/559/pull-descheduler-unit-test-master-master/1389200208998436864.

The log file shows all the places in my zz_generated.conversion files where there are errors, eg:

sigs.k8s.io/descheduler/pkg/api/v1alpha1/zz_generated.conversion.go:122:74: undefined: DeschedulerPolicy

You can see these errors yourself if you fetch my code and run ./hack/update-generated-conversions.sh, then open the sigs.k8s.io/descheduler/pkg/api/v1alpha1/zz_generated.conversion.go file.

So far I haven't been able to figure out the cause of these errors. If there is something I'm doing wrong, or something I should try, please let me know.

@RyanDevlin in that Prow output I see: sigs.k8s.io/descheduler/sigs.k8s.io/descheduler/pkg/api/v1alpha1, and from your other comment above (#559 (comment)) it sounds like the generator script is creating an entirely new directory called sigs.k8s.io/descheduler within the project?

If so, something is totally broken. The reason you're getting undefined errors is because those generated files don't have any type definitions to go along with them. In other words, that script should just be updating the existing zz_**.go files in the directories like https://github.com/kubernetes-sigs/descheduler/tree/master/pkg/api

So, first off we need to delete the sigs.k8s.io/descheduler directory that's being created, and then figure out why the script is creating a new one in the project root on your environment.

@RyanDevlin sorry for the late review. I finally found more time to check your changes more closely.

@ingvagabund Thank you for taking the time to review my PR! I've resolved all of your comments except a few. These are left open with questions above. Please let me know if there is anything I missed.

/retest

@ingvagabund have you had time to review my comments above? Specifically the second comment about strategies that filtered for node taints and affinity?

Just to reiterate, my line of thinking was that the goal of this feature was to get all strategies in line with a method for considering taints, affinity, etc. before descheduling. Some strategies already performed a subset of these checks before considering pods for descheduling.

In order to make this process more uniform, I removed some of the filtering for taints, etc. from nodeAffinity, topologySpread, and maybe one or two other strategies. I obviously never removed any filtering that was critical to the strategy (eg. the affinity filtering performed in nodeAffinity).

My assumption was that, if a user desired more advanced filtering, they would turn on the nodeFit feature in order to gain a closer to optimal eviction process.

/retest

I like the point you are making. I am in favor of simplifying the code. On the other hand, we can't change the default behavior just because of that. I'd rather wait for a release or two and make it enabled by default. Then eventually remove the gate completely.

@ingvagabund That's a fair point. In that case, I can revert any filtering changes I made to the strategies for now. Also I'll wait to hear from @damemi @lixiang233 @seanmalloy for extra thoughts on this before pushing anything.

For this type of follow up task, should I file a new issue so we can keep track of the future work?

For this type of follow up task, should I file a new issue so we can keep track of the future work?

I'd like to hear from other as well what they think about it. Filling a tracking issue sounds good if agreed.

I like the point you are making. I am in favor of simplifying the code. On the other hand, we can't change the default behavior just because of that. I'd rather wait for a release or two and make it enabled by default. Then eventually remove the gate completely.

Yeah, let's not change the default behavior right now. Tracking it over the next 2 releases to be enabled by default makes sense, and falls in line with upstream's featuregating approach

@ingvagabund @damemi Thanks for the input! I've restored the original functionality of the strategies. I also agree that the tests are becoming exceedingly complex, especially the e2e tests.

Lastly, I've opened issue #574 to track the removal of the optimization performed by this feature from the individual strategies.

Hopefully that covers everything for this PR, but if not, please comment and I will make changes!

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, RyanDevlin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/lgtm
Had a PTO the last week. Sorry for the delay :)

@RyanDevlin thank you for the patience.

@damemi @ingvagabund Thank you both for the review!